feat(webapp): mollifier API GET read-fallback — synthetic primitives + route wiring#3755
feat(webapp): mollifier API GET read-fallback — synthetic primitives + route wiring#3755d-cs wants to merge 12 commits into
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
31f4726 to
b05929b
Compare
1838229 to
af0aeeb
Compare
b05929b to
b89da52
Compare
0919f7a to
f36c576
Compare
74fdf6d to
c6fa61f
Compare
047b240 to
e57bc5e
Compare
Five Devin findings on PR #3755: - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer was assigning buffered.friendlyId to the FoundRun.id field. The id column on PG is the internal cuid; downstream log correlation reads taskRun.id and was getting the friendly token instead. Fixed to read the cuid that readFallback already derives via RunId.fromFriendlyId. - api.v1.runs.$runId.trace.ts buffered branch hardcoded isPartial: true. Cancelled is a terminal state — the sibling spans route and syntheticTrace already gate this on !isCancelled. Match. - synthesisePayload helper replaces the silent typeof === "string" coercion. Object-shaped payloads now JSON- stringify (matching how the trigger path would serialise them) with a warn log so format drift is visible. Truly unserialisable inputs fall back to "" with an error log instead of silently dropping. - syntheticRedirectInfo now uses deserialiseMollifierSnapshot (the webapp-side wrapper) instead of deserialiseSnapshot from the redis-worker package directly. Both share the same implementation today, but pinning the wrapper means the two read-side modules can't drift if the snapshot encoding ever changes (e.g. msgpack). - attempts route loader verifies the run belongs to the authenticated environment (PG-first, buffer fallback) before returning the parity-empty list. Other run-scoped endpoints (spans, trace, retrieve) 404 cross-env; matching that closes the exists-vs-doesn't-exist side channel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
242ba73 to
6a8404d
Compare
Five Devin findings on PR #3755: - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer was assigning buffered.friendlyId to the FoundRun.id field. The id column on PG is the internal cuid; downstream log correlation reads taskRun.id and was getting the friendly token instead. Fixed to read the cuid that readFallback already derives via RunId.fromFriendlyId. - api.v1.runs.$runId.trace.ts buffered branch hardcoded isPartial: true. Cancelled is a terminal state — the sibling spans route and syntheticTrace already gate this on !isCancelled. Match. - synthesisePayload helper replaces the silent typeof === "string" coercion. Object-shaped payloads now JSON- stringify (matching how the trigger path would serialise them) with a warn log so format drift is visible. Truly unserialisable inputs fall back to "" with an error log instead of silently dropping. - syntheticRedirectInfo now uses deserialiseMollifierSnapshot (the webapp-side wrapper) instead of deserialiseSnapshot from the redis-worker package directly. Both share the same implementation today, but pinning the wrapper means the two read-side modules can't drift if the snapshot encoding ever changes (e.g. msgpack). - attempts route loader verifies the run belongs to the authenticated environment (PG-first, buffer fallback) before returning the parity-empty list. Other run-scoped endpoints (spans, trace, retrieve) 404 cross-env; matching that closes the exists-vs-doesn't-exist side channel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
f4131eb to
d8f6cf7
Compare
6a8404d to
bc9f4e2
Compare
Five Devin findings on PR #3755: - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer was assigning buffered.friendlyId to the FoundRun.id field. The id column on PG is the internal cuid; downstream log correlation reads taskRun.id and was getting the friendly token instead. Fixed to read the cuid that readFallback already derives via RunId.fromFriendlyId. - api.v1.runs.$runId.trace.ts buffered branch hardcoded isPartial: true. Cancelled is a terminal state — the sibling spans route and syntheticTrace already gate this on !isCancelled. Match. - synthesisePayload helper replaces the silent typeof === "string" coercion. Object-shaped payloads now JSON- stringify (matching how the trigger path would serialise them) with a warn log so format drift is visible. Truly unserialisable inputs fall back to "" with an error log instead of silently dropping. - syntheticRedirectInfo now uses deserialiseMollifierSnapshot (the webapp-side wrapper) instead of deserialiseSnapshot from the redis-worker package directly. Both share the same implementation today, but pinning the wrapper means the two read-side modules can't drift if the snapshot encoding ever changes (e.g. msgpack). - attempts route loader verifies the run belongs to the authenticated environment (PG-first, buffer fallback) before returning the parity-empty list. Other run-scoped endpoints (spans, trace, retrieve) 404 cross-env; matching that closes the exists-vs-doesn't-exist side channel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
d8f6cf7 to
796a2c0
Compare
bc9f4e2 to
637e8c0
Compare
Five Devin findings on PR #3755: - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer was assigning buffered.friendlyId to the FoundRun.id field. The id column on PG is the internal cuid; downstream log correlation reads taskRun.id and was getting the friendly token instead. Fixed to read the cuid that readFallback already derives via RunId.fromFriendlyId. - api.v1.runs.$runId.trace.ts buffered branch hardcoded isPartial: true. Cancelled is a terminal state — the sibling spans route and syntheticTrace already gate this on !isCancelled. Match. - synthesisePayload helper replaces the silent typeof === "string" coercion. Object-shaped payloads now JSON- stringify (matching how the trigger path would serialise them) with a warn log so format drift is visible. Truly unserialisable inputs fall back to "" with an error log instead of silently dropping. - syntheticRedirectInfo now uses deserialiseMollifierSnapshot (the webapp-side wrapper) instead of deserialiseSnapshot from the redis-worker package directly. Both share the same implementation today, but pinning the wrapper means the two read-side modules can't drift if the snapshot encoding ever changes (e.g. msgpack). - attempts route loader verifies the run belongs to the authenticated environment (PG-first, buffer fallback) before returning the parity-empty list. Other run-scoped endpoints (spans, trace, retrieve) 404 cross-env; matching that closes the exists-vs-doesn't-exist side channel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
796a2c0 to
b139391
Compare
637e8c0 to
65219db
Compare
b139391 to
d153042
Compare
65219db to
ccdcd9c
Compare
Five Devin findings on PR #3755: - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer was assigning buffered.friendlyId to the FoundRun.id field. The id column on PG is the internal cuid; downstream log correlation reads taskRun.id and was getting the friendly token instead. Fixed to read the cuid that readFallback already derives via RunId.fromFriendlyId. - api.v1.runs.$runId.trace.ts buffered branch hardcoded isPartial: true. Cancelled is a terminal state — the sibling spans route and syntheticTrace already gate this on !isCancelled. Match. - synthesisePayload helper replaces the silent typeof === "string" coercion. Object-shaped payloads now JSON- stringify (matching how the trigger path would serialise them) with a warn log so format drift is visible. Truly unserialisable inputs fall back to "" with an error log instead of silently dropping. - syntheticRedirectInfo now uses deserialiseMollifierSnapshot (the webapp-side wrapper) instead of deserialiseSnapshot from the redis-worker package directly. Both share the same implementation today, but pinning the wrapper means the two read-side modules can't drift if the snapshot encoding ever changes (e.g. msgpack). - attempts route loader verifies the run belongs to the authenticated environment (PG-first, buffer fallback) before returning the parity-empty list. Other run-scoped endpoints (spans, trace, retrieve) 404 cross-env; matching that closes the exists-vs-doesn't-exist side channel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
0753300 to
a1e1ad8
Compare
Five Devin findings on PR #3755: - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer was assigning buffered.friendlyId to the FoundRun.id field. The id column on PG is the internal cuid; downstream log correlation reads taskRun.id and was getting the friendly token instead. Fixed to read the cuid that readFallback already derives via RunId.fromFriendlyId. - api.v1.runs.$runId.trace.ts buffered branch hardcoded isPartial: true. Cancelled is a terminal state — the sibling spans route and syntheticTrace already gate this on !isCancelled. Match. - synthesisePayload helper replaces the silent typeof === "string" coercion. Object-shaped payloads now JSON- stringify (matching how the trigger path would serialise them) with a warn log so format drift is visible. Truly unserialisable inputs fall back to "" with an error log instead of silently dropping. - syntheticRedirectInfo now uses deserialiseMollifierSnapshot (the webapp-side wrapper) instead of deserialiseSnapshot from the redis-worker package directly. Both share the same implementation today, but pinning the wrapper means the two read-side modules can't drift if the snapshot encoding ever changes (e.g. msgpack). - attempts route loader verifies the run belongs to the authenticated environment (PG-first, buffer fallback) before returning the parity-empty list. Other run-scoped endpoints (spans, trace, retrieve) 404 cross-env; matching that closes the exists-vs-doesn't-exist side channel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
597cce5 to
188b8c7
Compare
Five Devin findings on PR #3755: - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer was assigning buffered.friendlyId to the FoundRun.id field. The id column on PG is the internal cuid; downstream log correlation reads taskRun.id and was getting the friendly token instead. Fixed to read the cuid that readFallback already derives via RunId.fromFriendlyId. - api.v1.runs.$runId.trace.ts buffered branch hardcoded isPartial: true. Cancelled is a terminal state — the sibling spans route and syntheticTrace already gate this on !isCancelled. Match. - synthesisePayload helper replaces the silent typeof === "string" coercion. Object-shaped payloads now JSON- stringify (matching how the trigger path would serialise them) with a warn log so format drift is visible. Truly unserialisable inputs fall back to "" with an error log instead of silently dropping. - syntheticRedirectInfo now uses deserialiseMollifierSnapshot (the webapp-side wrapper) instead of deserialiseSnapshot from the redis-worker package directly. Both share the same implementation today, but pinning the wrapper means the two read-side modules can't drift if the snapshot encoding ever changes (e.g. msgpack). - attempts route loader verifies the run belongs to the authenticated environment (PG-first, buffer fallback) before returning the parity-empty list. Other run-scoped endpoints (spans, trace, retrieve) 404 cross-env; matching that closes the exists-vs-doesn't-exist side channel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
c5f92e8 to
fe633cd
Compare
…utes + align workerQueue default Addresses three Devin review findings on PR #3755: - api.v1.runs.\$runId.spans.\$spanId.ts and api.v1.runs.\$runId.trace.ts: the buffered-run response branch hardcoded isError:false and only checked CANCELED for isPartial, so a FAILED buffered run rendered as "still in progress" — SDK consumers would poll forever. Now derives both flags from CANCELED and FAILED, matching syntheticTrace.server.ts. - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer: workerQueue defaulted to "main" while syntheticSpanRun.server.ts uses "". The API response's `region` is sourced via `run.workerQueue || undefined`, so "main" was advertising a region the run hadn't yet been assigned to. Aligned to "" so unassigned buffered runs coerce to region: undefined. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5fd8611 to
ffeab61
Compare
cc42721 to
70611c2
Compare
Five Devin findings on PR #3755: - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer was assigning buffered.friendlyId to the FoundRun.id field. The id column on PG is the internal cuid; downstream log correlation reads taskRun.id and was getting the friendly token instead. Fixed to read the cuid that readFallback already derives via RunId.fromFriendlyId. - api.v1.runs.$runId.trace.ts buffered branch hardcoded isPartial: true. Cancelled is a terminal state — the sibling spans route and syntheticTrace already gate this on !isCancelled. Match. - synthesisePayload helper replaces the silent typeof === "string" coercion. Object-shaped payloads now JSON- stringify (matching how the trigger path would serialise them) with a warn log so format drift is visible. Truly unserialisable inputs fall back to "" with an error log instead of silently dropping. - syntheticRedirectInfo now uses deserialiseMollifierSnapshot (the webapp-side wrapper) instead of deserialiseSnapshot from the redis-worker package directly. Both share the same implementation today, but pinning the wrapper means the two read-side modules can't drift if the snapshot encoding ever changes (e.g. msgpack). - attempts route loader verifies the run belongs to the authenticated environment (PG-first, buffer fallback) before returning the parity-empty list. Other run-scoped endpoints (spans, trace, retrieve) 404 cross-env; matching that closes the exists-vs-doesn't-exist side channel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…utes + align workerQueue default Addresses three Devin review findings on PR #3755: - api.v1.runs.\$runId.spans.\$spanId.ts and api.v1.runs.\$runId.trace.ts: the buffered-run response branch hardcoded isError:false and only checked CANCELED for isPartial, so a FAILED buffered run rendered as "still in progress" — SDK consumers would poll forever. Now derives both flags from CANCELED and FAILED, matching syntheticTrace.server.ts. - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer: workerQueue defaulted to "main" while syntheticSpanRun.server.ts uses "". The API response's `region` is sourced via `run.workerQueue || undefined`, so "main" was advertising a region the run hadn't yet been assigned to. Aligned to "" so unassigned buffered runs coerce to region: undefined. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fe21821 to
4d5d0e0
Compare
bbf59b5 to
4a1bcfe
Compare
…+ route wiring Synthesise QUEUED/FAILED responses from the mollifier buffer when a TaskRun row hasn't landed in Postgres yet. Wires the synthesis into: - ApiRetrieveRunPresenter - v1 trace GET route - v1 spans GET route - attempts route gains a GET loader (fixes pre-existing Remix 400) Stacked on the trigger-time decisions PR. The readFallback infra itself lives on the trigger PR (consumed by IdempotencyKeyConcern); this PR adds the route-level synthetic-rendering primitives. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the ad-hoc \`as Record<string, unknown>\` + \`typeof === "string"\` checks in \`findBufferedRunRedirectInfo\` with a Zod \`safeParse\` against a schema for the subset of fields the redirect needs (envSlug / projectSlug / orgSlug / optional spanId). Wrong-typed or missing fields now collapse into a single parse-fail branch that logs the structured issue list and returns null. Adds a regression test for the structural-vs-typeof distinction: \`environment.slug: 42\` (number) is now rejected, where the previous \`typeof slug === "string"\` chain would silently accept any string- typed value but had no defence against shape drift in other fields. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…l enum \`SyntheticRun.machinePreset\` is a plain string sourced from the mollifier snapshot, but \`SpanRun.machinePreset\` is the typed \`MachinePresetName\` enum (micro / small-1x / small-2x / medium-1x / medium-2x / large-1x / large-2x). The direct assignment failed \`tsc --noEmit\` and CI typecheck. Validate via \`MachinePresetName.safeParse\` and collapse unknown values to \`undefined\` so a stale preset returned by the buffer doesn't bleed into the UI as a typed-but-unknown value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five Devin findings on PR #3755: - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer was assigning buffered.friendlyId to the FoundRun.id field. The id column on PG is the internal cuid; downstream log correlation reads taskRun.id and was getting the friendly token instead. Fixed to read the cuid that readFallback already derives via RunId.fromFriendlyId. - api.v1.runs.$runId.trace.ts buffered branch hardcoded isPartial: true. Cancelled is a terminal state — the sibling spans route and syntheticTrace already gate this on !isCancelled. Match. - synthesisePayload helper replaces the silent typeof === "string" coercion. Object-shaped payloads now JSON- stringify (matching how the trigger path would serialise them) with a warn log so format drift is visible. Truly unserialisable inputs fall back to "" with an error log instead of silently dropping. - syntheticRedirectInfo now uses deserialiseMollifierSnapshot (the webapp-side wrapper) instead of deserialiseSnapshot from the redis-worker package directly. Both share the same implementation today, but pinning the wrapper means the two read-side modules can't drift if the snapshot encoding ever changes (e.g. msgpack). - attempts route loader verifies the run belongs to the authenticated environment (PG-first, buffer fallback) before returning the parity-empty list. Other run-scoped endpoints (spans, trace, retrieve) 404 cross-env; matching that closes the exists-vs-doesn't-exist side channel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The synthetic SpanRun/trace builders for buffered runs hardcoded non-terminal state, so a CANCELED or FAILED buffered run rendered as a healthy in-progress run: - syntheticSpanRun: FAILED now maps to SYSTEM_FAILURE (matching ApiRetrieveRunPresenter.bufferedStatusToTaskRunStatus); isFinished is true for CANCELED/FAILED; isError is true for FAILED; the error block is synthesised as STRING_ERROR and statusReason carries the message. - syntheticSpanRun: drop the empty-string spanId/taskIdentifier relationship stubs (blank task name + misleading `?span=` jump) since the snapshot only carries friendly IDs. - syntheticTrace: FAILED now renders as an errored, non-partial, "failed" root span instead of executing/partial. CANCELED stays "completed", matching RunPresenter's derivation. - tests: cover the CANCELED and FAILED terminal paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…truction
Addresses the higher-confidence read-fallback review findings:
- attempts GET loader: rebuilt on createLoaderApiRoute so it matches the
sibling read routes — accepts JWTs with run/task/tag/batch resource
scoping (was bare authenticateApiRequest, rejecting PUBLIC_JWT and doing
no scope check), and 404s with `x-should-retry: true` so SDK pollers keep
retrying a not-yet-materialised run instead of giving up.
- batch reconstruction: the snapshot embeds the batch as `{ id, index }`
(engine.trigger shape), but readFallback read a non-existent flat
`batchId`, so SyntheticRun.batchId was always undefined. Read it from
`snapshot.batch.id` (the internal cuid). synthesiseFoundRunFromBuffer now
populates `batch` from it, and the spans/trace buffer-path authorization
pushes the batch resource — so batch-scoped JWTs authorise against
buffered runs and the retrieve response reports the correct batchId.
- metadata: coerce a non-string buffered metadata defensively (JSON
stringify + warn) instead of silently dropping to null, mirroring
synthesisePayload. In practice metadata is always a string, so this is a
no-op guard, but it surfaces format drift to ops.
- tests: cover batchId extraction from the nested batch object and its
absence for non-batched runs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comments referenced internal phase labels ("Phase A2", "Phase A5") from
the development plan rather than describing what the code does. Replaced
with self-contained prose; the surrounding explanations were already
correct and are preserved.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…derivation syntheticTrace.server.ts shipped without a test file; this adds one, covering the identity-field passthrough, taskIdentifier-and-spanId defaults, the three rootSpanStatus branches (QUEUED→executing, CANCELED→completed, FAILED→failed) with their isPartial/isError/isCancelled flags, the 1ms duration floor, rootStartedAt mapping, and the single-span trace shape (empty events/timelineEvents, empty linkedRunIdBySpanId, undefined overridesBySpanId/queuedDuration). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…utes + align workerQueue default Addresses three Devin review findings on PR #3755: - api.v1.runs.\$runId.spans.\$spanId.ts and api.v1.runs.\$runId.trace.ts: the buffered-run response branch hardcoded isError:false and only checked CANCELED for isPartial, so a FAILED buffered run rendered as "still in progress" — SDK consumers would poll forever. Now derives both flags from CANCELED and FAILED, matching syntheticTrace.server.ts. - ApiRetrieveRunPresenter.synthesiseFoundRunFromBuffer: workerQueue defaulted to "main" while syntheticSpanRun.server.ts uses "". The API response's `region` is sourced via `run.workerQueue || undefined`, so "main" was advertising a region the run hadn't yet been assigned to. Aligned to "" so unassigned buffered runs coerce to region: undefined. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…drop scaffolding GET on attempts Two related cleanups on the mollifier read surface: 1. Extract the buffered-run response bodies for the spans-detail and trace endpoints into pure helpers (apps/webapp/app/v3/mollifier/syntheticApiResponses.server.ts: buildSyntheticSpanDetailBody, buildSyntheticTraceBody). The route bodies were carrying the only copy of the terminal-state derivation (CANCELED / FAILED → isError / isPartial / isCancelled) with no unit coverage; extracting them lets us pin the contract directly. The route files now just authenticate, resolve, validate the spanId, and forward — no body shape logic in routes. 2. Drop the GET loader on api.v1.runs.\$runParam.attempts.ts. It was added in this PR solely to fix a pre-existing Remix "no loader" 400 on a URL no SDK consumer was actually calling, and to give the mollifier-parity script a stable assertion target. The detailed attempt list lives on the v3 retrieve endpoint — the GET was scaffolding rather than product surface, and Devin's review flagged it as such. Reverted to action-only. Tests: 16 cases in apps/webapp/test/mollifierSyntheticApiResponses.test.ts covering QUEUED / CANCELED / FAILED for each body, plus identity and default-field passthrough. Pins the FAILED-terminal-state regression that shipped briefly with isPartial:true. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…pin synthesise contract with regression tests synthesiseFoundRunFromBuffer hardcoded `scheduleId: null`, which dropped the schedule field from the retrieve-API response for any scheduled trigger that landed in the mollifier buffer. Scheduled triggers go through the same TriggerTaskService path as API triggers and the gate doesn't bypass them, so the snapshot does carry scheduleId; the synthesis was just throwing it away. Forward `buffered.scheduleId ?? null` so resolveSchedule() can hydrate the schedule object from PG (the Schedule row exists before the trigger fires). Exported synthesiseFoundRunFromBuffer + the FoundRun type from the presenter and added apps/webapp/test/mollifierSynthesiseFoundRun.test.ts (16 cases). The test file pins the snapshot→FoundRun mapping that previously had no direct coverage — the new scheduleId forwarding plus earlier-session regressions (batch reconstruction, workerQueue default "", FAILED→SYSTEM_FAILURE status mapping, STRING_ERROR shape, defensive metadata coercion, idempotency defaults, execution-state zero defaults). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4d5d0e0 to
9fd7957
Compare
Summary
Synthesise QUEUED/FAILED responses from the mollifier buffer when a TaskRun row hasn't landed in Postgres yet. Wires the synthesis into:
ApiRetrieveRunPresenterThe
readFallbackinfra itself lives on the trigger PR (consumed byIdempotencyKeyConcern); this PR adds the route-level synthetic-rendering primitives.Stacked on the replay PR.
Test plan